lsコマンドをオブジェクト指向で作成しました by s-tone-gs · Pull Request #12 · s-tone-gs/ruby-practices

s-tone-gs · 2025-12-03T07:59:56Z

File、Ls、MultiColumnLs、LongFormatLsという4つのクラスを定義して作成しました。
testディレクトリ以下にはテストを記述したファイルとテスト用のファイルがあります。

レビューお願いいたします。

yoshitsugu

コメントしました！

yoshitsugu · 2025-12-10T23:14:57Z

07.ls_object/file.rb

@@ -0,0 +1,61 @@
+# frozen_string_literal: true
+
+module My


あまり My という意味のないモジュール名は使わないほうがよいように思います。
たしかに、サンプルコードなどでは MyClass などの My という言葉がよく使われますが、これは「なんかなんでもいいので私がつくったクラス」くらいの意味で、サンプルコードとしての簡便さをとっただけであり、実務では使わないです。基本的に意味のある名前をつけましょう。

安易な命名でした。
FileMetadataに修正しました。

yoshitsugu · 2025-12-10T23:15:18Z

07.ls_object/file.rb

+
+    attr_reader :name
+
+    private_constant :FILE_TYPES, :PERMISSIONS


private_constant 使えていてよいですね 👍

yoshitsugu · 2025-12-10T23:18:21Z

07.ls_object/ls.rb

+opt.parse(ARGV)
+
+files = get_files(all, reverse)
+puts long_format ? LongFormatLs.generate(files) : MultiColumnLs.generate(files)


フォーマットというのは出力制御の一種であり、それを元にコマンドやアプリケーション全体が分岐している、というのはちょっと違和感がありますね。
一般的に、プログラムは入力 -> 演算 -> 出力の3の部分で説明されることが多いです。この出力のところだけが全体部分に染み出してきているように見えます。
出力パーツが変化しうるもの、というくらいで捉えてはどうでしょうか。

クラス構成を再考で修正しました。

yoshitsugu · 2025-12-10T23:30:08Z

07.ls_object/get_files.rb

+
+require_relative 'file'
+
+def get_files(all, reverse)


これだけ浮いているのもちょっと気になりますね。クラス設計は再度見直す必要がありそうです。
オブジェクト指向の設計で大事になってくるのは、何がコアとなるオブジェクトか、というところです。lsコマンドで一番重要な概念は何でしょう？たとえば、ファイルのメタデータ(ファイル名や作成日時など)をあつめたクラスをコアとして設計できそうではないでしょうか。
現状の My::File をたとえば FileMetadata などとリネームして、それをコアに据え、入力や出力は場合によって変わる、周辺的なもの、と考えましょう。そうすると

lsコマンドクラス
-> 入力を扱うクラス(今回だとオプションなど) -> FileMetadata
-> 出力を扱うクラス(今回だと標準出力) -> FileMetadata

みたいな依存関係にしておくと、FileMetadataは何にも依存していないので、コアとしてメンテしやすいクラスになりますし、もし出力がJSON形式になったとしても出力を扱うクラスを変更すればよいです。
このように考えてみてはどうでしょうか。

クラス構成を再考で修正しました。

yoshitsugu · 2025-12-10T23:35:37Z

07.ls_object/long_format_ls.rb

+  COLUMN_COUNT = 1
+  def initialize(files)
+    content_widths = calc_widths(files)
+    super(files, COLUMN_COUNT, content_widths)


ここでわざわざCOLUMN_COUNT=1でmatrixを使うのはかなり意味がないように思いますね。これもたぶん出力によって最初からわけてしまった影響だと思います。
表形式にしないといけないのは今回だと -l がない場合だけですよね。これは出力方式の関心事なので、 initialize で考慮しないといけない、というのはおかしい気がしますね。

クラス構成を再考にて修正しました。

yoshitsugu

コメントしました

yoshitsugu · 2025-12-18T23:48:34Z

07.ls_object/input_builder.rb

+require 'optparse'
+require_relative 'file'
+
+class InputBuilder


ちょっと名前が抽象的すぎるので、もう少し特定のものが想起されるようにしましょう。説明的ですがCommandLineArgumentsParserとかですかね。

クラスの役割を見直しにて修正しました。
また、コマンドライン引数をパースした結果filesが返ってくるというのは不自然であったため、ファイル取得のメソッドをクラスから外しました。

yoshitsugu · 2025-12-18T23:49:13Z

07.ls_object/ls.rb

+
+class Ls
+  def self.run
+    files_and_option = InputBuilder.build


files, show_in_long_format = ...
みたいに分割代入するとわかりやすいように思います

クラスの役割を見直しにて修正しました。

yoshitsugu · 2025-12-18T23:50:27Z

07.ls_object/input_builder.rb

+  def self.build
+    input_builder = new
+    all, reverse, long_format = input_builder.parse_options
+    files = FileMetadata.get_files(all, reverse)


オプションとファイル取得までまとめたクラスを作るのであればこのクラス内にget_filesがあるほうがよい気がします。

#12 (comment)
こちらで命名を修正した際、そもそも命名が曖昧になったのも引数のパースとファイル取得という別の役割のものが一つのクラスにまとまっていることが原因であると考え、ファイル取得をクラスから外しました。

get_filesはFileMetadataに持たせたまま、呼び出しをlsクラス内で行うようにしました。

yoshitsugu · 2025-12-18T23:51:09Z

07.ls_object/directory_content_output.rb

+    @long_format ? LongFormat.generate(@files) : MultiColumnFormat.generate(@files)
+  end
+
+  class MultiColumnFormat


クラスごとにファイルを分けたほうが見やすいです。

クラスごとにファイルを分割にて修正しました。

yoshitsugu · 2025-12-18T23:52:24Z

07.ls_object/file.rb

+
+  def initialize(name)
+    @name = name
+    @state = ::File.stat(name)


state ではなく stat ですね statistics の略だと思われます。

typoを修正
にて修正しました。
勘違いしておりました(;'∀')

yoshitsugu

コメントしました！

yoshitsugu · 2025-12-21T22:59:13Z

07.ls_object/file_metadata.rb

ファイル名はクラス名と揃えましょう。

修正しました。

yoshitsugu · 2025-12-21T23:02:15Z

07.ls_object/input_builder.rb

+    opt.parse(ARGV)
+    [all, reverse, long_format]
+  end
+end


結局オプションしか扱わない設計になったので、 CommandLineOptions くらいにしてもいいかなと思いました。
booleanの3つの値をそのまま返すと、自明でない順番に対する依存が発生します。最初がallというのはこのコードをちゃんと把握していないとわからないですよね。名前をつけたほうがよいです。
インスタンス変数で各オプションを持つようにして

def show_all? @all end

などとすると

options = CommandLineOptions.new options.show_all?

みたいにアクセスでき、よりわかりやすくなるかなと思います。

配列の順番に依存しないように修正にて対応しました。

yoshitsugu · 2025-12-21T23:04:34Z

07.ls_object/directory_content_output.rb

+require_relative 'multi_column_format'
+require_relative 'long_format'
+
+class DirectoryContentOutput


lsコマンドの実際の組み込み版を考えると、 ls hoge.rb みたいにファイル指定しての表示もできるので、必ずしもDirectoryが対象であるとは限りません。
FileListPrinter とかになるかなと思います。

出力クラスの命名を修正にて対応しました。

@yoshitsugu
フィヨルドの通知が死んでいるのでこちらでもメンションさせていただきます。
お時間あるときに確認お願いします。

yoshitsugu

OKです！

s-tone-gs added 8 commits December 3, 2025 16:51

オブジェクト指向でlsコマンドを作成

348d712

テストコードを作成

af023c0

メソッドの挙動と命名があっていなかったため修正

b5ae272

要素の幅とし要素間の空白の幅も含まれていたので分離

c78d1df

lオプションの役割と命名にずれがあったため修正

7add53d

親クラスのlsが具体的な値を持つのが不自然であったため修正

b28b6d4

モジュールで切り出していたメソッドがlsクラスに包括されると判断してリファクタリング

81cc5bf

命名の具体度を上げて修正

50704a0

yoshitsugu reviewed Dec 10, 2025

View reviewed changes

クラス構成を再考

5ccd7e7

yoshitsugu reviewed Dec 18, 2025

View reviewed changes

s-tone-gs added 4 commits December 19, 2025 10:22

typoを修正

b71f8f5

クラスごとにファイルを分割

97ebac8

クラスの役割を見直し

30c62c4

余計な空白を削除

cf1429c

yoshitsugu reviewed Dec 21, 2025

View reviewed changes

s-tone-gs added 4 commits December 22, 2025 08:21

出力クラスの命名を修正

1a45ee7

配列の順番に依存しないように修正

e054253

ファイル名とクラス名を一致させるよう修正

f2bb7fa

インスタンスの命名をクラスと一致させるように修正

1967c3d

yoshitsugu approved these changes Dec 24, 2025

View reviewed changes

Conversation

s-tone-gs commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yoshitsugu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-tone-gs Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yoshitsugu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-tone-gs Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-tone-gs Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yoshitsugu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-tone-gs Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yoshitsugu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

s-tone-gs commented Dec 3, 2025 •

edited

Loading

s-tone-gs Dec 17, 2025 •

edited

Loading

s-tone-gs Dec 19, 2025 •

edited

Loading

s-tone-gs Dec 19, 2025 •

edited

Loading

s-tone-gs Dec 22, 2025 •

edited

Loading